EmergencyReparentShard: skip zero-position candidates in errant GTID detection - #20831
Conversation
…detection A demoted primary with an empty GTID position but the max reparent journal count was kept as a candidate and its empty position added to the evidence used to check lagged candidates. Empty evidence corroborates nothing but disables the accept-when-nothing-to-compare path in FindErrantGTIDs, so a lagged replica's entire GTID set was flagged errant, potentially leaving the empty primary as the only candidate left to promote. Restore the IsZero skip to the top of the maxLenCandidates loop; the empty-shard initialization case is still covered by the all-zero early return. Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
vitess-bot
Bot
added
NeedsWebsiteDocsUpdate
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
timvaillancourt
added
Type: Bug
Component: VTOrc
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20831 +/- ##
===========================================
- Coverage 69.67% 68.64% -1.03%
===========================================
Files 1614 14 -1600
Lines 216793 2526 -214267
===========================================
- Hits 151044 1734 -149310
+ Misses 65749 792 -64957
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f226553a56
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ing the evidence tier A zero-position tablet holding the max reparent journal count previously left errant GTID detection with no evidence at all: the lagged candidates were accepted blindly and never reported as starved, so an errant lagged replica could survive detection and be elected. Dropping nil/zero-position candidates before the journal counts are gathered lets the remaining candidates form the evidence tier and corroborate each other Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8a6b51374
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
In go/vt/vtctl/reparentutil/emergency_reparenter.go:1339-1347, it seems like the all-zero shortcut still conflates a new shard with one whose GTID state was wiped on every reachable candidate. It returns before reading _vt.reparent_journal, so zero-position tablets with journal lengths 2 and 1 both remain eligible and findMostAdvanced can break the positional tie using promotion rules, potentially selecting the tablet that missed a promotion. A nonzero journal count means this is not the never-initialized case the shortcut is preserving. I think that we should only allow this shortcut when every candidate’s journal count is zero and fail closed otherwise, with a regression test for all-zero positions plus existing or inconsistent journal history. No?
…e max-journal candidates The reparent journal counts are now gathered for every candidate before errant GTID detection, including zero-position tablets whose journal rows survive a GTID wipe. The all-zero shortcut only treats a shard as uninitialized when every journal is empty too, and when no candidate with a real GTID position holds the max journal count, ERS fails closed: the surviving candidates provably missed a promotion whose content can no longer be proven. A missing reparent journal table on a tablet with no GTIDs counts as zero entries so a brand-new shard can still be initialized Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
In go/vt/vtctl/reparentutil/emergency_reparenter.go:1547-1555, I think the missing-sidecar tolerance still needs to be gated by shardNeverInitialized, not only by the tablet’s empty position. On an initialized shard, a wiped former primary can have both an empty GTID position and no _vt.reparent_journal; treating that unknown journal depth as zero lets a replica with a potentially older visible journal become the maximum evidence tier and be promoted.
…ble on a never-initialized shard A missing sidecar journal table on an initialized shard hides an unknown journal depth: converting it to zero entries would let a survivor with a potentially older visible journal form the evidence tier alone and be promoted. The tolerance now also requires the topology's never-initialized signal, so on an initialized shard the gather fails instead Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
|
@mattlord right again, fixed in d589308: the missing-sidecar tolerance is now gated on the topology's never-initialized signal too. On an initialized shard a missing journal table stays an error, since it hides an unknown journal depth. Added a regression test with a wiped former primary, missing sidecar, and a lower-journal survivor on an initialized shard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d589308eed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| var starved []string | ||
| validCandidates, starved, err = erp.findErrantGTIDs(ctx, validCandidates, stoppedReplicationSnapshot.statusMap, tabletMap, opts.WaitReplicasTimeout, failedEvidence) | ||
| validCandidates, starved, err = erp.findErrantGTIDs(ctx, validCandidates, stoppedReplicationSnapshot.statusMap, tabletMap, opts.WaitReplicasTimeout, failedEvidence, shardNeverInitialized) |
There was a problem hiding this comment.
In the normal ERS path, this reruns errant-GTID detection only over the post-wait validCandidates; the preceding failedEvidence loop preserves only non-zero GTID positions. If an all-zero-position tablet has _vt.reparent_journal rows but fails the relay-log wait while another zero-position peer succeeds, the failed tablet is deleted before findErrantGTIDs reads journals, so the survivor can be treated as a never-initialized shard and promoted even though reachable journal history proves a prior promotion. The new fail-closed rule needs to read/count zero-position failed waiters too, or fail closed before dropping them.
AGENTS.md reference: go/vt/vtctl/reparentutil/AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This one is real but very unlikely, so we're leaving it as-is. For a zero-position tablet to be a failed waiter, every candidate must be zero-position, and the never-initialized shortcut then only fires when the survivors' journals are empty and the shard record has no primary alias or term. Whatever promotion wrote the dropped tablet's journal rows also wrote the shard record, so reaching a promotion here additionally requires the topology to have been wiped or rebuilt. A tablet that fails its relay log wait is also indistinguishable from one that died seconds before ERS started, which would never have been a candidate at all: ERS can only weigh evidence from tablets that respond
On main this scenario promotes unconditionally, since the all-zero shortcut fires before the journals or the topology are consulted, so this PR strictly narrows the hole rather than introducing it
| #### <a id="ers-wiped-gtid-fail-closed"/>`EmergencyReparentShard` fails closed when GTID state was wiped</a> | ||
|
|
||
| `EmergencyReparentShard` now reads the reparent journal of every candidate before errant GTID detection, including candidates with an empty GTID position. A tablet with an empty position cannot corroborate evidence or be promoted over tablets with real history, but its surviving reparent journal rows still prove the shard has promotion history. When every tablet holding the deepest journal history has an empty position, including when every position is empty on a shard whose journal shows history, ERS now fails instead of promoting a candidate that provably missed a promotion. A shard with empty positions and empty journals everywhere, and whose topology has never recorded a primary, is still treated as uninitialized, where any candidate may become the first primary; on such a never-initialized shard, a missing sidecar reparent journal table on a tablet with no GTIDs counts as an empty journal, so ERS can still initialize a brand-new shard; on an initialized shard a missing journal table hides an unknown journal depth and fails the operation. | ||
|
|
||
| **Impact**: ERS invocations that previously promoted an empty-position tablet (discarding the shard's history) or elected a survivor missing a promotion now fail with a `FAILED_PRECONDITION` error naming the wiped tablets. Restore the GTID state or data of a wiped tablet before retrying; removing the wiped tablets from the shard instead would discard the missed promotion's transactions. | ||
|
|
There was a problem hiding this comment.
Is this a fix for a change that wasn't released yet? If so, then I don't think we need to mention it in the release notes (or merge it into the release notes of the actual change this is fixing).
There was a problem hiding this comment.
@arthurschreiber this is a fix for existing ERS logic, but in hindsight this change might be overkill for the changelog. But to play devil's advocate, a sharp-eyed user might notice the difference
I'm leaning towards dropping the note. Thoughts @mattlord?
There was a problem hiding this comment.
Yeah, I think we should drop it. Remember that this is not a changelog (that is done separately during the release as a list of all PRs) but instead a release summary (highlights). And the summary should not generally have bug fixes but kew new features, breaking changes, etc. So bug fixes like this should not clutter the summary/highlights.
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
The previous missing-sidecar concern is addressed for initialized shards, but I think one contradictory-state edge remains in go/vt/vtctl/reparentutil/emergency_reparenter.go:1547-1559. shardNeverInitialized suppresses ERNoSuchTable for an empty-position tablet even when another reachable candidate has a nonzero GTID position and a positive journal count. That MySQL evidence proves the shard has promotion history despite the empty topology record, so the missing tablet’s journal depth is still unknown. Mapping it to zero can make the visible journal-count-1 replica the maximum evidence tier and promote it even if the missing tablet formerly had deeper history.
I reproduced this on the current HEAD: findErrantGTIDs(..., shardNeverInitialized=true) returns nil and keeps the nonzero replica. I think that we should only tolerate a missing journal when topology says never initialized and allPositionsZero; the existing maxLen check can then reject any visible journal history. A mixed-state regression test should cover this. No?
It's a pretty distant edge case, but should be easy to address too. What do you think?
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
…ble when every position is empty Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
|
@mattlord fixed in 6f9489c0429a34adde116a92196045a01ea01ee2: the missing-journal tolerance is now gated on the topology's never-initialized signal and every position being empty, so a nonzero position anywhere fails the gather instead of letting the visible journal form the evidence tier. Mixed-state regression test added For context this edge came from the tolerance added in 8af8950 during review, so it never existed on |
| // A missing journal table is only tolerated when the topology says never initialized | ||
| // and no candidate has any GTIDs; a nonzero position anywhere proves history, so an | ||
| // unreadable journal depth must fail the gather | ||
| reparentJournalLen, err := erp.gatherReparenJournalInfo(ctx, validCandidates, tabletMap, waitReplicasTimeout, shardNeverInitialized && allPositionsZero) |
| }() | ||
| length, err = erp.tmc.ReadReparentJournalInfo(groupCtx, tabletMap[alias].Tablet) | ||
| if err != nil && tolerateMissingJournal { | ||
| // A brand-new shard has no sidecar tables yet: treat a missing journal |
There was a problem hiding this comment.
Each tablet should have the full set of sidecar tables once it's available. But this check also doesn't hurt anything and is a safeguard against that behavior changing or having a bug etc.
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
LGTM! Just the couple of comments inline. The one is just info, the other regarding the missing [t] in the function name we should clean up first.
Thanks, @timvaillancourt ! ❤️
arthurschreiber
left a comment
arthurschreiber
left a comment
There was a problem hiding this comment.
LGTM once the small nits Matt pointed out are fixed.
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
A shard whose every reachable candidate reports a zero GTID position (nil or a typed-but-empty set such as "MySQL56/") has never seen a promotion: it is being initialized. Such tablets have no _vt sidecar tables yet, so running errant-GTID detection reads a reparent journal that does not exist and fails ERS-for-initialization. Skip detection and accept all candidates in that case. This stands in for the upstream mechanism (allPositionsZero + shardNeverInitialized inside gatherReparentJournalInfo), which is entangled with an ERS-refactor chain this fork has deliberately NOT backported: vitessio#20578 (extraEvidence/starved), vitessio#20780 (explicit split-brain recovery, which introduced the allPositionsZero early-return), and vitessio#20831/e5255c93b4 (journal-missing tolerance). A TODO marks this for retirement once the fork upgrades past those PRs. This is a prerequisite for aligning FindPositionsOfAllCandidates with upstream's empty-GTID classification (which will classify a fresh shard as GTID-based). Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Description
This PR fixes a bug introduced in #20780, which moved the zero-position skip in
findErrantGTIDsin a way that let a tablet with an empty GTID position remain a promotion candidate. An empty position corroborates nothing, but its presence could flag a healthy replica's entire GTID set as errant; worst-case, ERS promotes the empty tablet and discards the shard's history 😱The fix skips zero-position candidates up front; a tablet with no GTIDs can't corroborate anyone and shouldn't be promoted over tablets with real history. Review feedback hardened this further: ERS now fails closed when every tablet holding the deepest reparent journal history has an empty position, and an all-empty shard is only treated as new when the topology agrees it was never initialized
This is very unlikely to be hit in practice, as a tablet only reports an empty position with intact data after something abnormal (a
RESET MASTER, a restore that died mid-way, etc). Also #20780 has not shipped in a release, so no released version of Vitess is exposedRelated Issue(s)
Follow-up to #20780
Checklist
Deployment Notes
This PR was written primarily by Claude Code, including this summary